fix(releases): Normalize both release values to equal one another #101184
Conversation
github-actions
bot
added
the
Scope: Backend
![semgrep-code-getsentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #101184 +/- ##
============================================
+ Coverage 66.36% 81.10% +14.73%
============================================
Files 8661 8673 +12
Lines 384271 384792 +521
Branches 24275 24275
============================================
+ Hits 255020 312082 +57062
+ Misses 128904 72363 -56541
Partials 347 347 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| "release": Release.objects.filter( | ||
| organization_id=release.organization_id, | ||
| version=current_release_version, | ||
| ).get(), |
There was a problem hiding this comment.
PR looks good to me. One small question about the extra query we have here to get the release that matches current_release_version--can/should we remove the need for this? Disadvantages are a little bit of extra latency and edge cases where get() raises Release.DoesNotExist.
We could do an easy fix in the semver path by returning current_release alongside current_release_version in get_current_release_version_of_group, then doing current_release or Release.objects.filter( here.
The non-semver path also queries the release and returns only the version (ugh), but the logic is buried fairly deep and uses caching so I understand if we don't want to change anything in this path. But it would be cleaner and save us a query if we change get_current_release_version_of_group to get_current_release_of_group altogether. If we want to merge this PR as is, I'm happy to investigate more and potentially write a followup ref PR that does this.
There was a problem hiding this comment.
I agree. Its not ideal. Definitely something we could address in a follow-up. For this PR, we have enough to worry about so we'll limit the blast radius.
On performance, yes its another query but its indexed so it will be fast. These things do add up so its best to not add queries too cavalierly. My long term goal is to refactor this code path entirely. The code, as is, is incomprehensible. An intermediate step might be the change you suggested or it might be something deeper. It just depends on the performance we measure in production and if its causing issues or not. If its not causing issues then we might focus our efforts on more substantial improvements.
There was a problem hiding this comment.
Sounds good. Was gonna ask how I can follow up on measuring perf in prod but I think your DD walkthrough is coming up anyway!
There was a problem hiding this comment.
Sentry's tracing product would be better! You can filter to the PUT request and find this span in there. You can see how the system performs in aggregate and determine if you need to make a perf change or not.
| "release": Release.objects.filter( | ||
| organization_id=release.organization_id, | ||
| version=current_release_version, | ||
| ).get(), |
There was a problem hiding this comment.
Bug: Unhandled Exception When Fetching Nonexistent Release
During issue resolution in semver projects, the code fetches a Release object using .get() with current_release_version. If current_release_version doesn't correspond to an existing Release in the database, this raises Release.DoesNotExist, causing an unhandled exception.
There was a problem hiding this comment.
True but since the current_release_version has already been shown to exist in the db the only way this could happen is if the release was deleted (which we don't allow). Technically this could raise but I'm not sure what I'd do with the exception anyway. Raising seems to be appropriate for now.
| release_ids = ( | ||
| GroupRelease.objects.filter( | ||
| group_id=group_id, | ||
| project_id=project_id, | ||
| ) | ||
| .distinct() | ||
| .values_list("release_id", flat=True) | ||
| ) |
There was a problem hiding this comment.
High severity vulnerability may affect your project—review required:
Line 445 lists a dependency (django) with a known High severity vulnerability.
ℹ️ Why this matters
Affected versions of django are vulnerable to Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection'). SQL injection in Django's ORM column aliases: when using QuerySet.annotate(), QuerySet.alias(), QuerySet.aggregate(), or QuerySet.extra() with dictionary expansion (**kwargs), the dictionary keys are used unescaped as SQL column aliases. On MySQL and MariaDB backends, an attacker who can influence those keys (for example, by passing a crafted dict of annotations) can inject arbitrary SQL into the generated query.
To resolve this comment:
Check if you are using Django with MySQL or MariaDB.
- If you're affected, upgrade this dependency to at least version 5.2.7 at uv.lock.
- If you're not affected, comment
/fp we don't use this [condition]
💬 Ignore this finding
To ignore this, reply with:
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
You can view more details on this finding in the Semgrep AppSec Platform here.
Normalize resolve-in-next-release release values between activity data and group resolution params. Also, fetches the latest semver release scoped to the packages present on the group.
Previously when resolving in the next semver release the
releasestored on the group resolution was the most recent time-ordered release. This PR changes that so we're setting the latest semver release as thereleasevalue.But some projects can have multiple packages. Either old packages no longer in use or concurrent packages with their own ordering characteristics. By filtering by the package we force the group resolution to only consider releases relevant to the problem being solved.